Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Yui button inspired style #10061

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

scherler
Copy link
Contributor

@scherler scherler commented Dec 16, 2024

Testing done

image

  • .jenkins-button--outline
    image
  • .jenkins-button--outline (on hover)

Proposed changelog entries

  • human-readable text

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@janfaracik
Copy link
Contributor

Heyo. The existing classes for Button should work for your primary/destructive use cases - see https://weekly.ci.jenkins.io/design-library/Buttons/

image

e.g. jenkins-button jenkins-button--primary jenkins-!-destructive-color and jenkins-button jenkins-button--primary jenkins-!-success-color

You'd want to add an 'outline' modifier though for the 'Analyse now' button, e.g. jenkins-button jenkins-button--outline unless the default button styling could work instead?

@scherler
Copy link
Contributor Author

@janfaracik makes a lot of sense and thanks for pointing it out (I will follow up with a sample in the design-lib) I renamed the outline example

@scherler
Copy link
Contributor Author

jenkinsci/design-library-plugin#360

@timja timja added web-ui The PR includes WebUI changes which may need special expertise rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Dec 17, 2024
@timja timja requested a review from janfaracik December 17, 2024 14:05
@@ -75,6 +75,35 @@
}
}

.jenkins-button--outline {
color: var(--btn-secondary-color) !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--btn-* vars are deprecated (YUI only), would just use var(--text-color) if it needs to be set.

.jenkins-button--outline {
color: var(--btn-secondary-color) !important;
border: 2px solid var(--btn-secondary-border);
font-weight: bold;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These props could be inherited from the base jenkins-button class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well but the original yui outline button had been bold, but if you want we can change it but IMHO it makes more sense bold.
image
vs
image


&:not(:disabled) {
&:hover {
border: 2px solid var(--accent-color) !important;
Copy link
Contributor

@janfaracik janfaracik Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't work if paired with colour modifiers. We've been using currentColor in our other button modifiers to inherit the colour from modifiers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm are you sure
image
however I added jenkins-button--outline[class*="color"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jenkins-button jenkins-button--outline jenkins-!-success-color
image
:hover
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya, I'm travelling today but I'll take a look as soon as I can :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good traveling and MCAAHNY

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, hope you had a good time too :)

image
.jenkins-button--outline {
  color: var(--color, var(--text-color-secondary)) !important;
  --button-background--hover: color-mix(in srgb, var(--color, var(--text-color-secondary)) 15%, transparent);
  --button-background--active: color-mix(in srgb, var(--color, var(--text-color-secondary)) 20%, transparent);
  --button-box-shadow--focus: color-mix(in srgb, var(--color, var(--text-color-secondary)) 10%, transparent);

  &::before {
    background: transparent;
    border: 2px solid var(--color, var(--text-color-secondary));
    opacity: 1 !important;
  }

  &:hover {
    &::before {
      background: var(--button-background--hover) !important;
    }
  }

  &:active {
    &::before {
      background: var(--button-background--active) !important;
    }
  }
}

.jenkins-button[class*="color"] {
  background: transparent;
  color: var(--color) !important;

  &::before {
    --button-background: currentColor; // changed this
    opacity: 0.1;
  }

How is something like this? The buttons styling is a little confusing (and more complex than it needs to be as modern CSS wasn't available when it was originally written, I do wanna tidy up this file)

I’m not totally sure about adding an outline button to core - I feel our default/secondary style already covers the same ground. Any thoughts on using it as a local class, using a prefix like {plugin-name}- instead of jenkins-?

Any thoughts @jenkinsci/sig-ux ?

@scherler scherler requested a review from janfaracik December 20, 2024 11:23
Copy link

@A1exKH A1exKH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scherler LGTM.

@krisstern
Copy link
Member

I’m not totally sure about adding an outline button to core - I feel our default/secondary style already covers the same ground. Any thoughts on using it as a local class, using a prefix like {plugin-name}- instead of jenkins-?

I tend to agree that we should use the outline button as a local class instead of core.

@uhafner
Copy link
Member

uhafner commented Dec 31, 2024

I’m not totally sure about adding an outline button to core - I feel our default/secondary style already covers the same ground. Any thoughts on using it as a local class, using a prefix like {plugin-name}- instead of jenkins-?

I tend to agree that we should use the outline button as a local class instead of core.

I also think that having too many styling options will confuse plugin authors and users. I think all existing styles have a semantic meaning now (default, secondary, destructive), and if the new style has no semantic meaning then we should not add it.

What is the reason for adding this new style? Or why does one of the existing buttons do not work? In jenkinsci/warnings-ng-plugin#1912 we also replaced the deprecated outlined YUI button with the Jenkins secondary button style which works quire well.

@daniel-beck daniel-beck added the needs-justification This PR lacks a motivation for the proposed change. label Jan 17, 2025
@scherler
Copy link
Contributor Author

I implemented the suggested modifications @janfaracik (I had to adopt them a bit). I will not die on the hill saying we need to have it; however, it is a nice addition and a one-to-one replacement of the YUI button we had until now. I understand that using the default or another is acceptable, but not the same as the default.

@timja
Copy link
Member

timja commented Jan 20, 2025

I implemented the suggested modifications @janfaracik (I had to adopt them a bit). I will not die on the hill saying we need to have it; however, it is a nice addition and a one-to-one replacement of the YUI button we had until now. I understand that using the default or another is acceptable, but not the same as the default.

Can you demonstrate how the existing buttons don't work for you please? i.e. could you add the existing buttons and we can see what it looks like / where any improvements could be made?

We've dropped YUI and having a one-to-one replacement with the same look and feel is not a design goal.

@scherler
Copy link
Contributor Author

To give an example of the different look and feel I attach some screen shots form our product and some OS plugins

before

image

after

image

or e.g. the OS plugins jenkinsci/copyartifact-plugin#246

image

...as said, this PR is a nice to have and I would love the outlined buttons only but in all my drop yui prs I did not used it.

@daniel-beck
Copy link
Member

I don't get this reason.

Yes, buttons look different now. This is intentional. There's nothing special about the forms you're showing here compared to elsewhere in Jenkins. In particular, you haven't demonstrated why the new buttons wouldn't work, and a different, second button style is needed. #10061 (comment) remains unanswered.

(FWIW I don't like the new buttons either and think the outline buttons look nicer, but that would be motivation for changing all buttons, or at least secondary ones, back to the outline style, not introducing a second style with no semantic meaning.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-justification This PR lacks a motivation for the proposed change. rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants